feat: add account_advisory backfill job - #2285
Conversation
Reviewer's GuideAdds a new one-off account advisory backfill job, wires it into the job runner and Kubernetes cronjob configuration, and introduces supporting cache-task orchestration and public advisory drift checking to validate the populated data. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The backfill goroutines don’t appear to honor any cancellation/timeout context, so consider threading a context through
BackfillAccountAdvisory/WithTxto allow the job to terminate promptly when the process receives a shutdown signal. - The fixed concurrency limit (
guard := make(chan struct{}, 4)) is hard-coded; consider making this configurable (e.g., via an environment variable) so you can tune throughput vs. DB load without code changes. - Per-account
LogInfocalls in the inner loop may generate very noisy logs for largerh_accounttables; you might want to log only on failures and periodically on progress (e.g., every N accounts) instead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The backfill goroutines don’t appear to honor any cancellation/timeout context, so consider threading a context through `BackfillAccountAdvisory`/`WithTx` to allow the job to terminate promptly when the process receives a shutdown signal.
- The fixed concurrency limit (`guard := make(chan struct{}, 4)`) is hard-coded; consider making this configurable (e.g., via an environment variable) so you can tune throughput vs. DB load without code changes.
- Per-account `LogInfo` calls in the inner loop may generate very noisy logs for large `rh_account` tables; you might want to log only on failures and periodically on progress (e.g., every N accounts) instead.
## Individual Comments
### Comment 1
<location path="tasks/caches/backfill_account_advisory.go" line_range="31" />
<code_context>
+
+ utils.LogInfo("accounts", len(rhAccountIDs), "starting account_advisory backfill")
+
+ guard := make(chan struct{}, 4)
+
+ for i, rhAccountID := range rhAccountIDs {
</code_context>
<issue_to_address>
**suggestion:** Make the backfill concurrency level configurable instead of hard-coded.
The guard channel currently hard-codes concurrency to 4, which may not suit all environments or align with other job settings. Please source this from configuration (or at least a shared constant) so it can be tuned without code changes and kept consistent with other concurrency controls.
Suggested implementation:
```golang
utils.LogInfo("accounts", len(rhAccountIDs), "starting account_advisory backfill")
guard := make(chan struct{}, accountAdvisoryBackfillConcurrency)
```
Please also:
1. Define a shared constant near the top of `tasks/caches/backfill_account_advisory.go` (or in a common config/constants file) like:
`const accountAdvisoryBackfillConcurrency = 4`
2. If your codebase has a central configuration mechanism (e.g., environment-driven or a config struct), consider wiring `accountAdvisoryBackfillConcurrency` from there instead of a literal, so this value can be tuned per environment.
</issue_to_address>
### Comment 2
<location path="tasks/caches/backfill_account_advisory.go" line_range="19-22" />
<code_context>
+
+func backfillAccountAdvisoryPerAccounts(wg *sync.WaitGroup) {
+ var rhAccountIDs []int
+ err := tasks.WithReadReplicaTx(func(tx *gorm.DB) error {
+ return tx.Table("rh_account").
+ Order("hash_partition_id(id, 128), id").
+ Pluck("id", &rhAccountIDs).Error
+ })
+ if err != nil {
</code_context>
<issue_to_address>
**suggestion (performance):** Loading all account IDs into memory at once may not scale well for large datasets.
This loads all rh_account IDs into a slice before starting any work, which can use a lot of memory and delay processing if the table is large. Consider batching (e.g., LIMIT/OFFSET, cursor/streaming, or server-side pagination) so the backfill can run incrementally with bounded memory.
Suggested implementation:
```golang
func backfillAccountAdvisoryPerAccounts(wg *sync.WaitGroup) {
const batchSize = 1000
guard := make(chan struct{}, 4)
offset := 0
for {
var rhAccountIDs []int
err := tasks.WithReadReplicaTx(func(tx *gorm.DB) error {
return tx.Table("rh_account").
Order("hash_partition_id(id, 128), id").
Limit(batchSize).
Offset(offset).
Pluck("id", &rhAccountIDs).Error
})
if err != nil {
utils.LogError("err", err, "unable to load rh_account IDs for account_advisory backfill")
return
}
if len(rhAccountIDs) == 0 {
break
}
utils.LogInfo(
"batch_size", len(rhAccountIDs),
"offset", offset,
"starting account_advisory backfill batch",
)
for i, rhAccountID := range rhAccountIDs {
guard <- struct{}{}
```
Because only part of the function body is visible, you need to:
1. Ensure that the existing per-account processing logic (the code that currently follows `guard <- struct{}{}` and uses `i` and `rhAccountID`) remains unchanged and now operates inside the per-batch inner loop.
2. Confirm that the loop and function are properly closed after your existing per-account logic.
3. Optionally, increment `offset` by the number of IDs processed in each batch (e.g., `offset += len(rhAccountIDs)` at the end of the `for { ... }` loop) if you want deterministic pagination; if you rely on a stable order and no concurrent inserts/deletes, you may instead switch to keyset pagination on `id` for better robustness.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2285 +/- ##
==========================================
- Coverage 59.12% 58.83% -0.30%
==========================================
Files 149 150 +1
Lines 9549 9597 +48
==========================================
Hits 5646 5646
- Misses 3311 3359 +48
Partials 592 592
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
b87ee57 to
3fb1216
Compare
3fb1216 to
a8195f1
Compare
a8195f1 to
994fe9b
Compare
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The backfill goroutine concurrency is hard-coded to 4; consider making this configurable (e.g., via environment or job config) so you can tune load on the database without code changes.
- The account_advisory backfill goroutines do not appear to honor context cancellation beyond the initial setup; consider wiring context into the per-account work so the job can be aborted cleanly if requested.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The backfill goroutine concurrency is hard-coded to 4; consider making this configurable (e.g., via environment or job config) so you can tune load on the database without code changes.
- The account_advisory backfill goroutines do not appear to honor context cancellation beyond the initial setup; consider wiring context into the per-account work so the job can be aborted cleanly if requested.
## Individual Comments
### Comment 1
<location path="tasks/caches/backfill_account_advisory.go" line_range="38-47" />
<code_context>
+ go func(i, rhAccountID int) {
</code_context>
<issue_to_address>
**suggestion (performance):** Per-account logging at info level in a tight loop may cause excessive log volume.
Per-account info-level logs before/after backfill and on drift check errors will scale with the number of accounts and can significantly increase log volume and overhead. Please consider lowering verbosity for per-account messages (e.g., debug or aggregated progress logs), while keeping actual error logs at info/error.
Suggested implementation:
```golang
err := tasks.WithTx(func(tx *gorm.DB) error {
utils.LogDebug("i", i, "rh_account_id", rhAccountID, "backfilling account_advisory")
return tx.Exec("SELECT backfill_account_advisory(?)", rhAccountID).Error
})
```
To fully implement the logging-verbosity suggestion, you should also:
1. Scan this file for other per-account `utils.LogInfo` calls (e.g., per-account "completed backfill" or drift-check logs) and down-level them to `LogDebug` or aggregate them into periodic summary logs.
2. Ensure that only actual errors (e.g., failed backfill, drift check failures) are logged with `LogInfo`/`LogError`, keeping the high-level batch start/end logs at info level as they are.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
994fe9b to
15c4dc1
Compare
15c4dc1 to
60bbec1
Compare
Summary
Adds
account_advisory_backfilljob to populate historical data inaccount_advisoryFollow-up
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Add a suspended, schedulable job to backfill historical data into the account_advisory table and perform advisory drift checks per account.
New Features:
Enhancements:
Tests: